Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop Container Component Statuses #121

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Sep 27, 2017

@simon3z simon3z requested review from moolitayer and cben September 27, 2017 12:25
@simon3z
Copy link
Contributor

simon3z commented Sep 27, 2017

@zakiva IIRC there's a BZ to link, right?

@zakiva
Copy link
Contributor Author

zakiva commented Sep 27, 2017

@zakiva IIRC there's a BZ to link, right?

Just added it :)

@zakiva zakiva force-pushed the drop_component_statuses branch 2 times, most recently from 87ec298 to e697d8c Compare September 27, 2017 13:11
@miq-bot
Copy link
Member

miq-bot commented Sep 27, 2017

Checked commit zakiva@e697d8c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All LGTM ✂️ 🔥 ✂️ 🔥 ✂️

Please describe in which order those should be merged.

{:name => 'persistent_volumes'}, {:name => 'persistent_volume_claims'},
# workaround for: https://github.com/openshift/origin/issues/5865
{:name => 'component_statuses', :default => []}
{:name => 'persistent_volumes'}, {:name => 'persistent_volume_claims'}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do you mind keeping the trailing comma? (not everyone likes them, your call)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it :)
(and it wasn't there in the first place)

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@moolitayer
Copy link

moolitayer commented Sep 28, 2017

AFAIU this patch should be safe to merge

@simon3z
Copy link
Contributor

simon3z commented Sep 28, 2017

AFAIU this patch should be safe to merge

@moolitayer feel free to merge (sync with @zakiva ) when ready if decoupled from the other PRs.

@zakiva
Copy link
Contributor Author

zakiva commented Sep 28, 2017

@moolitayer yeah this should be safe to merge now, actually, it has to be merged before we remove the class/table in the main and schema repos.
(UI also should be safe).

@moolitayer moolitayer merged commit a7e9595 into ManageIQ:master Sep 28, 2017
@moolitayer
Copy link

✂️ 🔥 ✂️ 🔥 ✂️

@moolitayer moolitayer added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 28, 2017
cben added a commit to cben/manageiq-providers-openshift that referenced this pull request Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants